Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust integration (experimental) #99

Merged
merged 8 commits into from
Sep 28, 2023

Conversation

SirVer
Copy link
Contributor

@SirVer SirVer commented Sep 15, 2023

This is using cxx.rs to wrap the framework library and calling it from Rust. This code is currently only supporting providing an interface to be implemented, i.e. no variables publish or receiving and no calling of other interfaces. Those features are straightforward, quick and easy to implement, but for now this is probably enough to iron out the integration questions.

See the everestrs/README.md for more details on the implementation.

Co-authored-by: Dima Dorezyuk ddo@qwello.eu

Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
@SirVer
Copy link
Contributor Author

SirVer commented Sep 22, 2023

All comments addressed. The cpp linter is wrong btw, module_id_ is used: https://github.com/sirver/everest-framework/blob/hrapp/initial_rust_support/everestrs/everestrs_sys/everestrs_sys.cpp#L58

@SirVer SirVer changed the title First support for Rust Rust integration (experimental) Sep 26, 2023
Comment on lines +12 to +17
OUTPUT ${output_dir}lib.rs.h ${output_dir}lib.rs.cc
COMMAND ${CXXBRIDGE} ${CMAKE_CURRENT_SOURCE_DIR}/${module_name}/src/lib.rs --header -o ${output_dir}lib.rs.h
COMMAND ${CXXBRIDGE} ${CMAKE_CURRENT_SOURCE_DIR}/${module_name}/src/lib.rs -o ${output_dir}lib.rs.cc
DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/${module_name}/src/lib.rs
COMMENT "Generating cxx for ${module_name}"
VERBATIM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • rather than calling the cxxbridge util here directly, I would call cxx_build::bridge from within the build.rs
  • I think, it should be the concern of this cxxrs.cmake script to figure out the path of CXXBRIDGE and not if its including script

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The first one is a chicken and egg though: You would need to run cargo build to get the cpp code, which you need to run cmake to build the static C library, which cargo needs to link against. I.e. you would need to run cargo for only the build.rs file (which I do not think is possible), then run cmake, then cargo again. I think doing it here is the only possibility.
  2. I do not understand this comment. Can you expand?

everestrs/CMakeLists.txt Show resolved Hide resolved
everest::log
)

install(TARGETS everestrs_sys LIBRARY)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't binary ship this glue code library, as it depends on auto-generated stuff (`rust/cxx.h'), which will only be generated, when its using crate is build.

Copy link
Contributor Author

@SirVer SirVer Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do understand that you do not like the installing piece, however the comment is not actionable to me, I do not know what to do instead: To build any crate using everestrs, this library must be findable. The only way I know to make it findable is by installing it, that seems the EVerest way to go. If you have a better suggestion, I would be happy to try it out, but likely in a followup MR.

everestrs/Cargo.lock Show resolved Hide resolved
everestrs/Cargo.toml Show resolved Hide resolved
everestrs/everestrs/src/lib.rs Outdated Show resolved Hide resolved
everestrs/everestrs/src/lib.rs Outdated Show resolved Hide resolved
everestrs/everestrs/src/lib.rs Outdated Show resolved Hide resolved
everestrs/everestrs/src/lib.rs Outdated Show resolved Hide resolved
everestrs/everestrs/src/schema/interface.rs Show resolved Hide resolved
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
@SirVer
Copy link
Contributor Author

SirVer commented Sep 28, 2023

Thanks for the approval. I have no rights to merge this though, who can help me with this?

@hikinggrass
Copy link
Contributor

Thanks for the approval. I have no rights to merge this though, who can help me with this?

I can merge this, one minor thing would be to run clang-format over the c++ code, we do have a dockerized version of the exact version we require here, but you can see the output as well in the Lint step of the github action.

One example command line would be: (maybe adding --user to provider your correct userid/groupid)

docker run --volume "$(pwd):/source" [ghcr.io/everest/everest-clang-format:latest](http://ghcr.io/everest/everest-clang-format:latest) -r /source --extensions "hpp,cpp" -e "/source/cache" -e "/source/build" -e "/source/build-cross" -e "/source/build-clang" -i

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass hikinggrass merged commit dafb3d9 into EVerest:main Sep 28, 2023
2 of 3 checks passed
@SirVer
Copy link
Contributor Author

SirVer commented Sep 28, 2023

@hikinggrass Thanks for doing the formatting for me. I tried myself and the container does not run for me (MacOS Arm64):

docker run --volume "$(pwd):/source" ghcr.io/everest/everest-clang-format:latest -r /source --extensions "hpp,cpp" -e "/source/cache" -e "/source/build" -e "/source/build-cross" -e "/source/build-clang" -i
qemu-x86_64: Could not open '/lib64/ld-linux-x86-64.so.2': No such file or directory
run-clang-format: error: Command '['clang-format', '--version']' returned non-zero exit status 255.

It seems the linking inside the container is not done correctly, though I am confused by the qemu-x68_64

@SirVer SirVer deleted the hrapp/initial_rust_support branch September 28, 2023 17:58
@hikinggrass
Copy link
Contributor

@hikinggrass Thanks for doing the formatting for me. I tried myself and the container does not run for me (MacOS Arm64):

docker run --volume "$(pwd):/source" ghcr.io/everest/everest-clang-format:latest -r /source --extensions "hpp,cpp" -e "/source/cache" -e "/source/build" -e "/source/build-cross" -e "/source/build-clang" -i
qemu-x86_64: Could not open '/lib64/ld-linux-x86-64.so.2': No such file or directory
run-clang-format: error: Command '['clang-format', '--version']' returned non-zero exit status 255.

It seems the linking inside the container is not done correctly, though I am confused by the qemu-x68_64

Ah that actually makes sense, since our container was initially just meant for the github action it just uses a prebuilt clang-format binary for x86-64. That then obviously doesn't work on arm64

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants